Skip to content

ARROW-1290: [C++] Double buffer size when exceeding capacity in arrow::BufferBuilder as in array builders #905

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wesm wants to merge 1 commit into from
Closed

ARROW-1290: [C++] Double buffer size when exceeding capacity in arrow::BufferBuilder as in array builders #905

wesm wants to merge 1 commit into from

Conversation

@wesm
Copy link
Member

@wesm wesm commented Jul 28, 2017 •
edited
Loading

Kind of an embarrassing oversight, but it's good that we caught it.

In a test for incrementally building a BinaryArray, this yields about 4x speedup

Benchmark                                     Time           CPU Iterations
---------------------------------------------------------------------------
BM_BuildBinaryArray/repeats:3             11892 us      11892 us         59   840.886MB/s
BM_BuildBinaryArray/repeats:3             11903 us      11904 us         59   840.082MB/s
BM_BuildBinaryArray/repeats:3             11909 us      11910 us         59   839.662MB/s
BM_BuildBinaryArray/repeats:3_mean        11902 us      11902 us         59    840.21MB/s
BM_BuildBinaryArray/repeats:3_stddev          7 us          7 us          0   520.137kB/s

before:

Benchmark                                     Time           CPU Iterations
---------------------------------------------------------------------------
BM_BuildBinaryArray/repeats:3             45678 us      45571 us         15   219.439MB/s
BM_BuildBinaryArray/repeats:3             45416 us      45209 us         15   221.197MB/s
BM_BuildBinaryArray/repeats:3             45227 us      45122 us         15   221.619MB/s
BM_BuildBinaryArray/repeats:3_mean        45440 us      45301 us         15   220.752MB/s
BM_BuildBinaryArray/repeats:3_stddev        185 us        194 us          0   966.716kB/s

@wesm
Double buffer size when exceeding capacity in arrow::BufferBuilder, l…
59d4d9c 
…ike in other array builder classes

Change-Id: Ibb9328dc9482ce334c4f434f7479079414f5e301
@jeffknupp
Copy link
Contributor

jeffknupp commented Jul 28, 2017

@wesm yes, I'll test both with and without jemalloc

@jeffknupp
Copy link
Contributor

jeffknupp commented Jul 28, 2017

Certainly fixed in the non-jemalloc case. Going to test with jemalloc 4.5.0 now.

@jeffknupp
Copy link
Contributor

jeffknupp commented Jul 28, 2017

The jemalloc issue remains (which makes sense, since it's happening in a single threaded process and all this change does is reduce the frequency of reallocations)

xhochy
xhochy approved these changes Jul 28, 2017
View reviewed changes
Copy link
Member

@xhochy xhochy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM

@asfgit asfgit closed this in 4df2a0b Jul 28, 2017
@wesm wesm deleted the ARROW-1290 branch July 29, 2017 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xhochy xhochy xhochy approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@wesm @jeffknupp @xhochy